Skip to content

Conditional rendering testing value of object of forward/backward link#201

Open
jg10-mastodon-social wants to merge 36 commits into
mainfrom
feat/conditional-values
Open

Conditional rendering testing value of object of forward/backward link#201
jg10-mastodon-social wants to merge 36 commits into
mainfrom
feat/conditional-values

Conversation

@jg10-mastodon-social

@jg10-mastodon-social jg10-mastodon-social commented Apr 18, 2026

Copy link
Copy Markdown
Collaborator

Closes #184

  • Add some-value-eq and every-value-eq
  • Add Thing.observeLiterals
  • Support literals for if-property
  • Add (some|every)-value-(gt|gte|lt|lte)
  • Tests have been written
    • all new code is covered by unit tests
    • the happy path of a new feature is covered by an end-to-end test
      • added to pos-switch integration test
    • manual explorative tests have been performed
  • all dependencies are updated to the latest patch version at minimum
  • the CI pipeline passes
  • documentation is up-to-date
    • TSDoc style comments on important functions, properties and events
    • stories for new PodOS elements have been added to storybook
    • Readme.md files of PodOS elements have been re-generated
    • N/A- architectural decisions are documented as an ADR
    • Changelogs are updated according to Keep a Changelog

`);
});

it('does not render templates when compareValue indicates that some-value-(lt|lte|gt|gte) is not met (numeric)', async () => {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we might need more tests here, but I'm not sure how to design them.
I designed these as TDD tests - they all fail if the numbers are treated as strings.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now grouped tests, and split out tests specifically of the test method.
I've tried to systematically go through possible combinations of inputs and data, using the it.each approach.

The code is still missing systematic tests of:

  • evaluating values of property and rev with multiple relations
  • evaluating values of property with multiple strings
  • evaluating values of property with multiple numbers

There are ad-hoc tests for this functionality, so given the difficulty in parameterising these combinatorial tests, I suggest we open an issue documenting this gap and merge in the mean time.

Any suggestions are welcome for systematically testing all combinations in the presence of multiple values!

Comment thread elements/src/components/pos-switch/pos-switch.spec.tsx Outdated
Comment thread core/CHANGELOG.md
and this project adheres to
[Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## 0.30.0

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now 0.30.0, but I may have misunderstood what is in the previous release

Comment thread elements/CHANGELOG.md

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## 0.42.0

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angelo-v Is this correct?
When rebasing on main, it looked like your previous fixes no longer applied given the release in the mean time?

@angelo-v angelo-v left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jg10-mastodon-social Sorry, I just noticed I left some comments in "pending" state and you propbably did not see them. I am just submitting them now - yet I did not find the time to look at your latest changes, so I am not sure if they are still relevant. I am going to continue reviewing the current state later this week

expect(page.root?.innerText).toEqualText('Video.');
});

it.each([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: lets cover all cases as data-driven tests like that

issue: the commented out test cases are failing

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should now be fully addressed

let state = null;
let values = null;

const compareValue = function (values: string[]): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: this function is very complex and should a) be refactored into smaller chunks and b) moved into its own file and heavily unit tested. Comparision logic be separated from element attributes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't seen this comment and have therefore not refactored. Any help in dividing it up would be great!

I did however create a separate test file for this function so (b) is partially addressed.

I'm not sure about separating out comparison logic from element attributes - the main purpose of the function is to apply all necessary combinations of element attributes - the comparison logic itself is arguably trivial once the effect of the element attributes is applied.

}
if (caseElement.getAttribute('if-property') !== null) {
state = this.relations.map(x => x.predicate).includes(caseElement.getAttribute('if-property'));
const matchingRelations = this.relations.filter(x => x.predicate == caseElement.getAttribute('if-property'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: same as above. Try to collect the relevant data and condition from component first, then do the logic in a dedicated function that is independent from the component and well tested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Conditional rendering testing value of object of forward/backward link

2 participants